Skip to content

Use raw pointer for NUL-terminated file location with #[track_caller] #142579

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

Darksonn
Copy link
Contributor

@Darksonn Darksonn commented Jun 16, 2025

As a follow-up to the Location::file_with_nul ACP, I'm filing this PR to ask a question for T-libs-api:

Should the NUL-terminated file location returned by core::panic::Location use the &CStr type or the raw pointer type?

This question is listed as an unresolved question in the ACP and tracking issue.

The main advantage of a raw pointer is that creating a &CStr requires knowing the length. This means that if we reintroduce the optimization from #117431 of not storing the length anywhere, callers of file_with_nul will make an unnecessary call to strlen when they just need to pass the pointer into C code. The main disadvantage is that using the raw pointer is unsafe.

Tracking issue: #141727

@Darksonn Darksonn added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. A-panic Area: Panicking machinery A-rust-for-linux Relevant for the Rust-for-Linux project labels Jun 16, 2025
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 16, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 16, 2025

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@a1phyr
Copy link
Contributor

a1phyr commented Jun 17, 2025

callers of file_with_nul will make an unnecessary call to strlen when they just need to pass the pointer into C code.

I don't think that this is really a disadvantage, as the compiler is perfectly able to remove an unnecessary call to strlen (eg see https://godbolt.org/z/3ezaz1esa).

@m-ou-se
Copy link
Member

m-ou-se commented Jun 17, 2025

There is still a plan to make &CStr a thin pointer that does not know the string length. It's just blocked on figuring out how to make !Sized types more flexible.

@m-ou-se
Copy link
Member

m-ou-se commented Jun 17, 2025

We discussed this in the libs-api meeting. There wasn't much enthusiasm for this change.

Part of the reason is that we might still make &CStr a thin pointer. Even if we don't, using a raw pointer here is a very inconvenient type. Reducing the size of Location by not storing the length doesn't seem to have much advantages, and does mean having to use strlen for the regular file() -> &str method. And even if we do end up going that route (while keeping &CStr wide), the strlen call can be easily optimized away as pointed out above.

Because of these reasons, I'm closing this PR.

@m-ou-se m-ou-se closed this Jun 17, 2025
@Darksonn
Copy link
Contributor Author

Thanks! Marking the feature as implementation complete in the tracking issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-panic Area: Panicking machinery A-rust-for-linux Relevant for the Rust-for-Linux project I-libs-api-nominated Nominated for discussion during a libs-api team meeting. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants